I7 parameters #14

eyvorchuk · 2020-07-30T23:20:25Z

This PR solves issue 7, which is to wrap the parameters module into a process, test it with pytest, and show it in a demo notebook. The only things that need to be done are to properly incorporate wps_tools (currently in progress) and test it using opendap files.

into i7-parameters

eyvorchuk · 2020-08-11T18:54:05Z

I just added some commits to get the test and demo working. Could you guys try running them and let me know if there are any problems? Currently, the pytest will only work if called from the root osprey directory because the sample_parameter_config.cfg file uses file paths that are relative to that directory. You also might have to make the modification to share.py that Sangwon mentioned in his PR.

eyvorchuk · 2020-08-11T19:02:59Z

Also change pour_points.ix to pour_points.loc in rvic/parameters.

sum1lim

pytest and demo-run are all successful except for test_wps_caps. Nice work!

sum1lim · 2020-08-11T20:14:24Z

osprey/processes/wps_convolution.py

+import os
+import logging
+import json
+


Could we use wps_tools utils and io like you did in wps_parameters?

I'm gonna save that for a different PR since Nik already assigned me a separate issue for that.

sum1lim · 2020-08-11T20:18:49Z

osprey/processes/wps_parameters.py

+        if request.inputs["version"][0].data:
+            from rvic import version


The import statement in wps_convolution is placed at the top. Should we be matching the place it is imported?

sum1lim · 2020-08-11T20:27:49Z

osprey/processes/wps_parameters.py

+            process_step="process",
+            log_level=loglevel,
+        )
+        parameters(config, np)


While is good for now to pass config filepath right into RVIC parameters, I think it would be nice to implement a pathway for calling the function with a dictionary argument depending on the RVIC version. We could probably do this later in another PR. I will leave that decision up to you.

I'll add this once your config_builder PR is done.

sum1lim · 2020-08-11T20:32:41Z

tests/test_wps_caps.py

@@ -11,5 +10,5 @@ def test_wps_caps():
        "/wps:Capabilities" "/wps:ProcessOfferings" "/wps:Process" "/ows:Identifier"
    )
    assert sorted(names.split()) == [
-        "hello",
+        "convolution",


Add parameters here

sum1lim · 2020-08-11T20:39:49Z

tests/data/samples/sample_parameter_config.cfg

+#-- Path to Pour Points File (char) --#
+# A comma separated file of outlets to route to [lons, lats] - one coordinate pair per line (order not important)
+# May optionally include a column [names] - which will (if not aggregating) be included in param file
+FILE_NAME: tests/data/samples/sample_pour.txt


Unlike RVIC convolution that uses a huge dataset, I think it is pretty reasonable to test RVIC parameters on local files. Good choice!

sum1lim · 2020-08-11T20:41:20Z

tests/test_wps_parameters.py

+from osprey.processes.wps_parameters import Parameters
+
+
+@pytest.mark.parametrize(("config"), [resource_filename(__name__, "data/samples/sample_parameter_config.cfg")])


It would be nice to include a test case for opendap as well.

Like, file paths inside the config file are opendap? I think I can make that work, but I'd have to delete a statement from rvic/parameters.py which I'm not sure is needed.

Could you elaborate more about deleting a statement in rvic/parameters.py? Which line are you referring to?

So, unlike in rvic/convolution.py, rvic/parameters.py calls copy_inputs to copy the inputs to the CASEDIR. In doing so, it calls the copyfile function for each file path in the config file. Since the opendap urls don't exists as local files, this won't work. If I get rid of the initial call to copy_inputs, this won't be a problem, but I'd be removing functionality. The thing is, since this isn't done in rvic/convolution.py, I don't know how necessary this statement is.

Hmm, that is very interesting. So none of POUR_POINTS, UH_BOX, ROUTING, and DOMAIN supports opendap input? I think at least DOMAIN has to be taken from the server for proper use.

From my experience, only POUR_POINTS should be taken from user's local directory. @corviday What do you think?

nikola-rados

Some small changes but I think this pretty much ready to go. I would wait to hear from @corviday before merging though.

nikola-rados · 2020-08-11T22:12:58Z

.github/workflows/python-ci.yml

-    - name: Test with pytest (full)
-      if: github.ref == 'refs/heads/master'
-      run: |
-        py.test -v
-    - name: Test with pytest (fast)
-      if: github.ref != 'refs/heads/master'
-      run: |
-        py.test -m "not slow" -v


I'd much rather these were just commented out rather than removed.

nikola-rados · 2020-08-11T22:15:10Z

osprey/processes/wps_parameters.py

+        if request.inputs["version"][0].data:
+            from rvic import version


Good point @sum1lim, it would be nice to have the files matching. I believe having the imports at the top is standard so we can move them there.

eyvorchuk · 2020-08-11T23:00:30Z

Could you guys try running test_wps_parameters, once from the root osprey directory and once somewhere else? I modified the test to use a temporary config file which should make it more flexible.

corviday · 2020-08-12T22:43:25Z

Running test_wps_parameters from the root directory:

(venv) lzeman@pcic-2023:~/tmp/osprey-parameters/osprey$ pytest tests/test_wps_parameters.py 
=================================================================== test session starts ====================================================================
platform linux -- Python 3.6.9, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
rootdir: /home/lzeman/tmp/osprey-parameters/osprey, configfile: setup.cfg
plugins: notebook-0.6.0, nbval-0.9.6, flake8-1.0.6
collected 1 item                                                                                                                                           

tests/test_wps_parameters.py F                                                                                                                       [100%]

========================================================================= FAILURES =========================================================================
_____________________ test_parameters_local[/home/lzeman/tmp/osprey-parameters/osprey/tests/data/samples/sample_parameter_config.cfg] ______________________
Traceback (most recent call last):
  File "/home/lzeman/tmp/osprey-parameters/osprey/tests/test_wps_parameters.py", line 23, in test_parameters_local
    run_wps_process(Parameters(), params)
  File "/home/lzeman/tmp/osprey-parameters/osprey/venv/lib/python3.6/site-packages/wps_tools/testing.py", line 65, in run_wps_process
    assert_response_success(resp)
  File "/home/lzeman/tmp/osprey-parameters/osprey/venv/lib/python3.6/site-packages/pywps/tests.py", line 149, in assert_response_success
    assert len(success) == 1
AssertionError

running it from the osprey directory:

(venv) lzeman@pcic-2023:~/tmp/osprey-parameters/osprey/osprey$ pytest ../tests/test_wps_parameters.py 
=================================================================== test session starts ====================================================================
platform linux -- Python 3.6.9, pytest-6.0.1, py-1.9.0, pluggy-0.13.1
rootdir: /home/lzeman/tmp/osprey-parameters/osprey, configfile: setup.cfg
plugins: notebook-0.6.0, nbval-0.9.6, flake8-1.0.6
collected 1 item                                                                                                                                           

../tests/test_wps_parameters.py F                                                                                                                    [100%]

========================================================================= FAILURES =========================================================================
_____________________ test_parameters_local[/home/lzeman/tmp/osprey-parameters/osprey/tests/data/samples/sample_parameter_config.cfg] ______________________
Traceback (most recent call last):
  File "/home/lzeman/tmp/osprey-parameters/osprey/tests/test_wps_parameters.py", line 23, in test_parameters_local
    run_wps_process(Parameters(), params)
  File "/home/lzeman/tmp/osprey-parameters/osprey/venv/lib/python3.6/site-packages/wps_tools/testing.py", line 65, in run_wps_process
    assert_response_success(resp)
  File "/home/lzeman/tmp/osprey-parameters/osprey/venv/lib/python3.6/site-packages/pywps/tests.py", line 149, in assert_response_success
    assert len(success) == 1
AssertionError

eyvorchuk · 2020-08-12T22:58:32Z

@corviday Did you make this modification to rvic/core/share.py and this modification to rvic/parameters.py?

corviday · 2020-08-13T16:21:46Z

I didn't know about the second set of modifications. After I made those, pytest test_wps_parameters.py worked from multipl directories.

eyvorchuk · 2020-08-13T17:06:28Z

@corviday Is the PR ready for merging?

corviday

I played around with the notebook, changing the pour point and looking at the results. The results seem good to me.

sum1lim and others added 30 commits July 16, 2020 09:14

add wps_Convolution.py

20421cb

wps_convolution added to __init__.py

b22ac3d

add rvic to requirements.txt

fd035f7

add demo_convolution

ebc4d28

Add wps_parameters.py

1d872bd

wps_convolution takes .cfg input and produces .nc output

7e6d7f5

use helper-functions of convolution rather than calling convolution()

3ca2375

dynamic determination of output filepath

1d9eadc

back to using convolution()

0b4a2cc

Add test_wps_parameters.py and sample parameters

e96411c

modification to fit RVIC 1.1.0.post1

5ed025d

fixed bug in outputting

c6efd85

config files in tests/data/configs

20799ad

add test_wps_convolution.py

de8c13d

dynamic definition for config filepath

b1487e8

run_wps_process moved to common.py

b1764e9

config input is a str not a file

daab9b0

delete unnecessary test file

092de15

test_wps_convolution marked as slow and online

b31f57e

black applied on common.py

fd5f6a8

move demo_requirements to requirements_dev

a04add0

revert test_wps_caps.py

decb3a5

delete wps_say_hello

4df6c62

different pathways depending on rvic version

51b99c6

pin rvic version

9f874ce

use lower case letters for variables

9b3dcd8

logging out messages

98102d1

set curr_dir to osprey

0afd3a0

Add rvic and notebook requirements to requirements files

0c35042

Update imported module names in wps_parameters

0f4ae91

Eric Yvorchuk added 9 commits August 4, 2020 09:10

Update imported module names in wps_parameters

1976e9f

Only use local config file in pytest

36f5359

Add demo notebook

1364423

Replace convolution with parameters in comment

59d6417

Format test_wps_parameters with black

73aac17

Merge branch 'i7-parameters' of https://github.com/pacificclimate/osprey

22e3bb2

into i7-parameters

Use json.loads instead of eval and use fewer percentage steps

fd1e6dc

Update wps_parameters to use wps-tools

065a8a6

Show successful parameters demo and move to demo_individual directory

6dbed3a

eyvorchuk marked this pull request as ready for review August 11, 2020 18:50

sum1lim suggested changes Aug 11, 2020

View reviewed changes

Eric Yvorchuk added 3 commits August 11, 2020 13:50

Remove pytest from CI workflow

5482670

Format with black

d94c5db

Add parameters to test_wps_caps

2f04700

This was linked to issues Aug 11, 2020

Wrap RVIC parameters module into wps #7

Closed

Make running test_wps_parameters.py more flexible #22

Closed

nikola-rados approved these changes Aug 11, 2020

View reviewed changes

Eric Yvorchuk added 3 commits August 11, 2020 15:20

Comment out instead of remove pytest in CI workflow

2a7c0ef

Move version import to top of file

52c68c2

Use temporary config file for wps_parameters

497e5a3

corviday approved these changes Aug 13, 2020

View reviewed changes

eyvorchuk merged commit ca1175d into master Aug 13, 2020

eyvorchuk deleted the i7-parameters branch August 13, 2020 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I7 parameters #14

I7 parameters #14

eyvorchuk commented Jul 30, 2020

eyvorchuk commented Aug 11, 2020

eyvorchuk commented Aug 11, 2020

sum1lim left a comment

sum1lim Aug 11, 2020

eyvorchuk Aug 11, 2020

sum1lim Aug 11, 2020

sum1lim Aug 11, 2020

eyvorchuk Aug 11, 2020

sum1lim Aug 11, 2020

sum1lim Aug 11, 2020

sum1lim Aug 11, 2020

eyvorchuk Aug 11, 2020

sum1lim Aug 11, 2020

eyvorchuk Aug 11, 2020

sum1lim Aug 11, 2020

sum1lim Aug 11, 2020 •

edited

Loading

nikola-rados left a comment

nikola-rados Aug 11, 2020

nikola-rados Aug 11, 2020

eyvorchuk commented Aug 11, 2020

corviday commented Aug 12, 2020

eyvorchuk commented Aug 12, 2020

corviday commented Aug 13, 2020

eyvorchuk commented Aug 13, 2020

corviday left a comment

		if request.inputs["version"][0].data:
		from rvic import version

		from osprey.processes.wps_parameters import Parameters


		@pytest.mark.parametrize(("config"), [resource_filename(__name__, "data/samples/sample_parameter_config.cfg")])

I7 parameters #14

I7 parameters #14

Conversation

eyvorchuk commented Jul 30, 2020

eyvorchuk commented Aug 11, 2020

eyvorchuk commented Aug 11, 2020

sum1lim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sum1lim Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

nikola-rados left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyvorchuk commented Aug 11, 2020

corviday commented Aug 12, 2020

eyvorchuk commented Aug 12, 2020

corviday commented Aug 13, 2020

eyvorchuk commented Aug 13, 2020

corviday left a comment

Choose a reason for hiding this comment

sum1lim Aug 11, 2020 •

edited

Loading